Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: PoC of inter-realm spec #2958

Open
wants to merge 94 commits into
base: master
Choose a base branch
from

Conversation

ltzmaxwell
Copy link
Contributor

@ltzmaxwell ltzmaxwell commented Oct 16, 2024

address: the inter realm spec part from #2743.

Please refer to the original doc, and #2743 (comment)

And please see the comments below for details of implementation .

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

@ltzmaxwell ltzmaxwell requested review from jaekwon, moul, piux2, thehowl, mvertes and a team as code owners October 16, 2024 10:22
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related labels Oct 16, 2024
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 77.55102% with 55 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/realm.go 69.93% 41 Missing and 5 partials ⚠️
gnovm/pkg/gnolang/ownership.go 95.45% 2 Missing and 1 partial ⚠️
gnovm/pkg/gnolang/store.go 0.00% 2 Missing and 1 partial ⚠️
gnovm/pkg/gnolang/uverse.go 40.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@jefft0 jefft0 added review/triage-pending PRs opened by external contributors that are waiting for the 1st review and removed review/triage-pending PRs opened by external contributors that are waiting for the 1st review labels Oct 16, 2024
@jefft0
Copy link
Contributor

jefft0 commented Oct 17, 2024

(The "review team" label was added by mistake. This PR is opened by a core dev.)

@@ -341,7 +341,7 @@ func main() {
// "Base": {
// "@type": "/gno.RefValue",
// "Escaped": true,
// "ObjectID": "336074805fc853987abe6f7fe3ad97a6a6f3077a:2"
// "ObjectID": "purePkg:336074805fc853987abe6f7fe3ad97a6a6f3077a:2"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for object from pure package, make objectID with a prefix "purePkg", so it's possible to distinct with object from realm. this does not change the current realm object persistence. but it looks ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thehowl any idea for this?

Copy link
Contributor Author

@ltzmaxwell ltzmaxwell Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe r:xxx and p:xxx is good.

@ltzmaxwell ltzmaxwell changed the title WIP fix(gnovm): PoC of inter-realm spec feat (gnovm): PoC of inter-realm spec Feb 16, 2025
@ltzmaxwell ltzmaxwell marked this pull request as ready for review February 16, 2025 07:31
@ltzmaxwell ltzmaxwell changed the title feat (gnovm): PoC of inter-realm spec feat: PoC of inter-realm spec Feb 16, 2025
@ltzmaxwell ltzmaxwell requested a review from thehowl February 16, 2025 07:33
Comment on lines +1 to +30
// PKGPATH: gno.land/r/crossrealm_test
package crossrealm_test

import (
crossrealm "gno.land/r/demo/tests/crossrealm"
)

type container struct {
name string
f *fooer
}

func (container) Foo() { println("hello " + c.f.name) }

type fooer struct{ name string }

var c = &container{name: "local_container"}

func main() {
ff := &fooer{name: "local_foo"}
c.f = ff
crossrealm.SetFooer(c)
crossrealm.CallFooerFoo()
print(".")
}

// Output:
// hello local_foo
// .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test demonstrates how the scenario functions:
image

the local_container will be firstly attached by reference to the external realm, after the external realm finalization, the embedding &fooer will be attached to current realm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @moul

Comment on lines +1 to +29
// PKGPATH: gno.land/r/crossrealm_test
package crossrealm_test

import (
crossrealm "gno.land/r/demo/tests/crossrealm/func"
)

var b = 1

type Local_S struct {
name string
}

func main() {
f := func() bool {
c := b
var ls Local_S
return true
}
crossrealm.SetCallback(f)
println("ok")
}

// Output:
// ok

// Realm:
// switchrealm["gno.land/r/demo/tests/crossrealm/func"]
// switchrealm["gno.land/r/crossrealm_test"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see this discussion about FuncValue association .
#2958 (comment)

@Kouteki Kouteki requested a review from mvertes February 17, 2025 10:43
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've spent most of today looking through realm.go and ownership.go so I could understand how they worked; I made a commit adding a bunch of comments which I hope are useful to whoever tries to understand it next.

I made some preliminary comments; I'll need another pass to be critical and proactive about different approaches / solutions.

gnovm/pkg/gnolang/ownership.go Outdated Show resolved Hide resolved
Comment on lines 174 to 180
// realm where object is from
originRealm PkgID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we used a *PackageValue here instead? Could it not avoid us having to put whether it's a pure package in the PkgID?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this is a bit weird, because "from" makes it sound like it's about where it's declared; while really it's the type's package where possible. Could you also update it?

I can also try to make a better comment on these if you prefer, but if you make a first pass I think it's better ;)

// realm where object is from
originRealm PkgID

// if this object is attaching as a base of reference
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would appreciate a better comment here, not quite clear what this is for. Can you write an example?

gnovm/pkg/gnolang/realm.go Outdated Show resolved Hide resolved
@@ -220,9 +264,127 @@ func (rlm *Realm) DidUpdate(po, xo, co Object) {
}
}

// checkCrossRealm2 checks cross realm recursively.
func checkCrossRealm2(rlm *Realm, store Store, tv *TypedValue, isLastRef bool, seenObjs []Object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have tests checking for some kinds of circular references?

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, was meant to be just a "comment". Putting request changes for now.

@ltzmaxwell ltzmaxwell force-pushed the dev/maxwell/cross-realm branch from 1a6f3b9 to 49eb014 Compare February 18, 2025 15:30
Comment on lines +1 to +17
// PKGPATH: gno.land/r/crossrealm_test
package crossrealm_test

import (
crossrealm "gno.land/r/demo/tests/crossrealm/array"
)

var root interface{}

func main() {
root = crossrealm.GetArray5()
println(".")
}

// Output:
// .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works even the result of GetArray5() is already real. the value is copied to current realm, and it's not bound to external realm, because its Type is not a declared type bound to the external realm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

7 participants